-
-
Notifications
You must be signed in to change notification settings - Fork 147
refactor: #718 only drop TimestampSeries #1274
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
c81cd6e
to
d5e1089
Compare
d5e1089
to
41c7015
Compare
abf9147
to
cbbd372
Compare
@cmp0xff you have a number of PRs submitted while I was out on vacation for 2 weeks. Can you let me know which ones I should prioritize for review? |
Hi @Dr-Irv, I hope you had a nice vacation. My pull requests are categorised below. Each category is independent, but those in a higher position have a slightly higher priority in my opinion.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for doing this. It's a lot of good work.
Main thing - if I'm going to merge this PR, it needs to be in a state where we don't need the followup PR.
Basic rule - we don't put ignore
in the tests unless we are testing that the stubs should not accept something that is invalid. You have places where you have added ignore
in the tests and I won't merge that in (unless we know it is a bug in the type checker)
Thank you very much for your quick and thorough reviews. I will be able to work on them next week. |
cbbd372
to
ed69ec5
Compare
b095af2
to
f1cf19f
Compare
Before making this PR as "ready for review", I probably can still clean up the |
tests/series/arithmetic/test_sub.py
Outdated
check(assert_type(left_ts.rsub(s), pd.Series), pd.Series, pd.Timedelta) | ||
check(assert_type(left_ts.rsub(a), pd.Series), pd.Series, pd.Timedelta) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
datetime - Series[Any]
can either be timedelta
-like or datetime
-like, depending on Any
. I would not give an exact type here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, that makes sense.
) -> TimestampProperties: ... | ||
@overload | ||
def __get__( | ||
self, instance: Series[Timedelta], owner: Any |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Use TimedeltaSeries
here or TimedeltaSeries | Series[Timedelta]
and then change in the PR that will remove TimedeltaSeries
pandas-stubs/core/series.pyi
Outdated
def __mul__( | ||
self, other: timedelta | Timedelta | TimedeltaSeries | np.timedelta64 | ||
self: Series[bool], | ||
other: timedelta | np.timedelta64 | np_ndarray_td | TimedeltaSeries, | ||
) -> TimedeltaSeries: ... | ||
@overload | ||
def __mul__(self: Series[bool], other: Series[Timedelta]) -> Series[Timedelta]: ... # type: ignore[overload-overlap] | ||
@overload | ||
def __mul__( | ||
self: Series[int], | ||
other: timedelta | np.timedelta64 | np_ndarray_td | TimedeltaSeries, | ||
) -> TimedeltaSeries: ... | ||
@overload | ||
def __mul__(self: Series[int], other: Series[Timedelta]) -> Series[Timedelta]: ... | ||
@overload | ||
def __mul__( | ||
self: Series[float], | ||
other: timedelta | np.timedelta64 | np_ndarray_td | TimedeltaSeries, | ||
) -> TimedeltaSeries: ... | ||
@overload | ||
def __mul__(self: Series[float], other: Series[Timedelta]) -> Series[Timedelta]: ... |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it possible to combine these overloads like this?
@overload
def __mul__(
self: Series[bool] | Series[int] | Series[float],
other: timedelta | np.timedelta64 | np_ndarray_td | TimedeltaSeries,
) -> TimedeltaSeries: ...
@overload
def __mul__(self: Series[bool] | Series[int] | Series[float],, other: Series[Timedelta]) -> Series[Timedelta]: ... # type: ignore[overload-overlap]
def median( | ||
self, | ||
self: Series[float], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
self: Series[float], | |
self: Series[float] | Series[int] |
And then for Series[bool]
, median()
returns np.floating
@overload | ||
def to_numpy( | ||
self, | ||
dtype: DTypeLike | None = None, | ||
copy: bool = False, | ||
na_value: Scalar = ..., | ||
**kwargs, | ||
) -> np_1darray: ... |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think you need this overload because IndexOpsMixin
has it. Then you can get rid of the ignore
s in _SeriesSubClassBase
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looked at the most recent version, and found one issue, and there are a few other issues open from previous review. Ping when you want me to look at it.
# checking, where our `__radd__` cannot override. At runtime, they return | ||
# `Series`s. | ||
if TYPE_CHECKING_INVALID_USAGE: | ||
assert_type(i + left, "npt.NDArray[np.int64]") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
any assert_type()
within the if TYPE_CHECKING_INVALID_USAGE
has to have a # type: ignore
in it.
If it fails at runtime, and we can't detect it (which is the case here, I think), then just comment out the test and include a comment indicating why this can't be tested.
Hi @Dr-Irv , before trying to resolve the comments, I would like to ask your opinion on the progress. In this MR we need As for now, all tests for What do you think? Shall we continue with the current effort, at the price of maybe dropping the |
A couple of questions:
|
Yes, the errors are
Let's say we have the following two as the first two overloads of @overload
def __sub__(
self: Series[Timestamp], other: Series[Any]
) -> Never: ...
@overload
def __sub__(
self: Series[Timestamp], other: datetime | np.datetime64 | np_ndarray_dt
) -> TimedeltaSeries: ...
|
OK, so I say leave in the tests, put in a Do the same issues occur with
OK, so much for that idea. I think I understand why. |
Hi @Dr-Irv, after some thoughts, my feeling now is that such behaviour is a Consider the following example: from typing import Any, overload, Generic, reveal_type
from typing_extensions import TypeVar, Never, Self
T = TypeVar("T", bound=int, default=Any)
class Se(Generic[T]):
@overload
def __sub__(self: Se[int], other: Se[int]) -> Never: ...
@overload
def __sub__(self, other: Self) -> Self: ...
def foo(a: Se[int]) -> Se[int]: ...
reveal_type(foo(Se[Any]())) # mypy: Se[int]; pyright: Se[int], no typing error
reveal_type(Se[Any]() - Se[Any]()) # mypy: Any, pyright: Never The first If we agree with what I wrote above, the two overloads of What do you think? |
I think that the typing spec is unclear with respect to how overloads get matched with generic types and defaults. If you have a variable that has type Conversely, if you have a variable that has type I don't think that Consider this modification of your example, where I took out the default value of the from __future__ import annotations
from typing import (
Any,
overload,
Generic,
Never,
reveal_type,
Self,
Any,
)
from typing_extensions import TypeVar
T = TypeVar("T", int, str)
class Se(Generic[T]):
@overload
def __sub__(self: Se[int], other: Se[int]) -> Never: ...
@overload
def __sub__(self, other: Self) -> Self: ...
def __sub__(self, other: Any) -> Self:
return self
def foo(a: Se[int]) -> Se[int]:
return Se[int]()
def unknown(a: list[Se]) -> Se:
reveal_type(a[0])
return a[0]
def t1() -> None:
seany = Se[Any]()
reveal_type(seany)
fooseany = foo(seany)
reveal_type(fooseany)
sub = seany - Se[Any]()
reveal_type(sub)
def t2() -> None:
seunk = unknown([Se[Any]()])
reveal_type(seunk)
foosunk = foo(seunk)
reveal_type(seunk)
sub = seunk - seunk
reveal_type(sub) With Here's something to try. What if we created a class class PUnknown:
pass Then the |
Let me take your example, but restrict to the case I think we are really concerned with. from __future__ import annotations
from typing import (
Any,
overload,
Generic,
Never,
reveal_type,
Self,
)
from typing_extensions import TypeVar
T = TypeVar("T", int, str)
class Se(Generic[T]):
@overload
def __sub__(self: Se[int], other: Se[int]) -> Never: ...
@overload
def __sub__(self, other: Self) -> Self: ...
def __sub__(self, other):
return self
def foo(a: Se[int]) -> Se[int]:
return Se[int]()
def t1() -> None:
seany = Se[Any]()
sub = seany - Se[Any]()
reveal_type(sub) # mypy: Any, pyright: Never.
# Mypy thinks both Se[int] - Se[int] -> Never and Self - Self -> Self apply. The result is ambiguous, so mypy gives Any.
# Pyright does not take into account the second overload. I believe this is essentially the same as the example 4 in the python typing documentation for overload, extracted below: @overload
def example4(x: list[int], y: int) -> int: ...
@overload
def example4(x: list[str], y: str) -> int: ...
@overload
def example4(x: int, y: int) -> list[int]: ...
def test(v1: list[Any], v2: Any):
# Step 2 eliminates the second overload. Step 5
# determines that first and third overloads
# both apply and are ambiguous due to Any, and
# the return types are inconsistent.
r2 = example4(v2, 1)
reveal_type(r2) # Should reveal Any Above, What I want to argue is that |
Yes, but I think we can get around this by never having a type corresponding to |
If I understand correctly, we can do the experiment below: (it's an from typing import Any, overload, Generic, reveal_type
from typing_extensions import TypeVar, Never, Self
class PUnknown: ...
T = TypeVar("T", bound=int | PUnknown, default=PUnknown)
class Se(Generic[T]):
@overload
def __sub__(self: Se[int], other: Se[int]) -> Never: ...
@overload
def __sub__(self, other: Self) -> Self: ...
def foo(a: Se[int]) -> Se[int]: ...
reveal_type(foo(Se[PUnknown]())) # mypy, pyright: cannot assign
reveal_type(Se[PUnknown]() - Se[PUnknown]()) # mypy, pyright: Se[PUnknown] I think it will be a big change, worthy a separate PR, if we do it. In particular, |
I played with the idea. Seems like a lot of work. The subtype argument is what makes it a problem. So let's not go down that path.
I actually don't think that pyright is wrong by saying I wish the typing spec allowed you to have
I see your point. So where does that leave us? What are our options? This might be the argument to keep |
The following is my understanding:
|
TimestampSeries
,TimedeltaSeries
, etc. can be removed #718assert_type()
to assert the type of any return value